CHANGELOG.md:12

• Changelog entries in this file consistently include an issue/PR reference link (e.g. "(#4950)"). This new entry is missing a reference, which makes it harder to trace the change back to the discussion/PR.

* Strengthen TRE API authentication: introduce layered `auth/` package with typed exceptions, `PyJWKClient`-backed token validation with issuer checking, immutable `AuthenticatedUser` model, and composable RBAC factories; remove the `AccessService` abstraction that is no longer needed now that Entra ID is the only auth provider.

CHANGELOG.md:12

• The new CHANGELOG entry does not include an issue/PR reference link, while other entries in this section consistently include one (e.g. "(#4920)"). The contribution guidelines in this repo require changelog entries to include an issue and/or PR reference.

* Strengthen TRE API authentication: introduce layered `auth/` package with typed exceptions, `PyJWKClient`-backed token validation with issuer checking, immutable `AuthenticatedUser` model, and composable RBAC factories; remove the `AccessService` abstraction that is no longer needed now that Entra ID is the only auth provider.

api_app/auth/dependencies.py:17

• 401 responses raised from the auth dependency currently omit the WWW-Authenticate: Bearer header. The previous auth layer included this header on 401s, and omitting it can break standards-compliant clients and auth middleware that relies on it.

def _to_http_exception(exc: AuthError) -> HTTPException:
    if isinstance(exc, TokenExpired):
        return HTTPException(
            status_code=status.HTTP_401_UNAUTHORIZED,
            detail=strings.EXPIRED_SIGNATURE,

.flake8:4

• This PR changes the global Flake8 ignore list to include E231. Disabling E231 repo-wide reduces lint signal outside the specific false-positive cases and was previously called out as something we should avoid changing globally. Prefer fixing/annotating the offending lines (e.g., targeted # noqa: E231) or pinning/upgrading the linter instead of turning the rule off for the entire codebase.

# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501, W503, E231

.github/linters/.flake8:4

• This PR adds E231 to the global ignore list for the GitHub workflow Flake8 config. This disables a general whitespace rule across the entire repo for CI, which can mask real formatting issues unrelated to the Python 3.12/pycodestyle false positives described in the comment. Consider addressing the specific violations instead of broadening the global ignore list.

# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501,W503,E231

api_app/event_grid/helpers.py:46

• publish_event ends with raise last_exc # type: ignore[misc]. This relies on a type-ignore to silence the possibility of raising None, which makes the failure path harder to reason about and can hide real bugs if the retry loop logic changes. It’s cleaner to ensure last_exc is non-None before raising.

    raise last_exc  # type: ignore[misc]

api_app/_version.py:1

• PR description says api_app/_version.py was bumped to 0.26.0, but this change sets it to 0.26.1. Please align the implementation and PR notes/versioning intent so releases and changelog entries match the actual version being shipped.

__version__ = "0.26.1"

.flake8:4

• Ignoring E231 globally disables a useful formatting check across the entire repo and can mask real whitespace issues unrelated to the pycodestyle f-string tokenisation bug mentioned in the comment. Prefer scoping this to the specific affected files/lines via per-file-ignores or targeted # noqa: E231, rather than adding E231 to the global ignore list.

[flake8]
# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501, W503, E231

.github/linters/.flake8:4

• Same concern as the root .flake8: adding E231 to the global ignore list here broadens the workaround to the entire codebase and can hide real formatting issues. Prefer scoping the ignore to the specific affected files/lines via per-file-ignores (or local # noqa: E231) so E231 remains enforced elsewhere.

[flake8]
# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501,W503,E231

api_app/auth/models.py:30

• AuthenticatedUser.email is declared as Optional[str] = None, but downstream models require a string email (e.g. AirlockNotificationUserData.email: str in models/domain/events.py). This PR also relies on empty-string email to avoid 503s when email claims are missing; allowing None here makes it easy to reintroduce the issue when AuthenticatedUser is constructed without an email (e.g. tests_ma/test_api/conftest.py currently does). Consider making email a plain string with an empty-string default.

    id: str
    name: str
    email: Optional[str] = None
    roles: Tuple[str, ...] = Field(default_factory=tuple)

api_app/_version.py:1

• PR description notes api_app/_version.py was bumped to 0.26.0, but this change sets it to 0.26.2. Please align the PR description and the version bump (either update the description or adjust the version) so reviewers and release tooling have a consistent reference.

__version__ = "0.26.2"

.flake8:4

• Avoid globally ignoring flake8 E231. This suppresses a real formatting check across the whole repo (and both local + CI linter configs), and the PR thread indicates we shouldn’t change global lint settings. Prefer a targeted workaround (per-file-ignores or inline # noqa: E231) limited to the few files/lines affected by the pycodestyle/Python 3.12 false positives.

[flake8]
# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501, W503, E231

.github/linters/.flake8:4

• Avoid globally ignoring flake8 E231 in the CI linter config as well. Disabling E231 repo-wide can hide real formatting issues; use per-file-ignores or # noqa: E231 only where the false positives occur.

[flake8]
# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501,W503,E231

api_app/services/airlock.py:301

• Similarly, returning the raw Event Grid exception text to callers (EVENT_GRID_PUBLISH_FAILED.format(e)) increases information disclosure risk. If the goal is to distinguish transient failures, consider returning a stable message and logging e (or include only a safe error code) instead of echoing the exception string.

    except Exception as e:
        await airlock_request_repo.delete_item(airlock_request.id)
        logger.exception("Failed sending status_changed message")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.EVENT_GRID_PUBLISH_FAILED.format(e))

api_app/auth/models.py:33

• AuthenticatedUser.email is typed as Optional and defaults to None, but downstream models (e.g. AirlockNotificationUserData.email) require a string; allowing None makes it easy for callers/tests to create users that later fail validation and surface as 5xx errors. Prefer making email a non-optional string with an empty-string default (the token validator already normalises missing claims to "").

    id: str
    name: str
    email: Optional[str] = None
    roles: Tuple[str, ...] = Field(default_factory=tuple)
    audience: str = ""
    is_workspace_token: bool = False

api_app/tests_ma/test_api/conftest.py:28

• The global no_auth_token fixture builds an AuthenticatedUser without an email. Since some downstream payloads validate email as a required string, it’s safer for this default test user to match production normalisation (missing claim -> empty string) and avoid accidental None propagation in tests.
  api_app/_version.py:1
• The PR description says api_app/_version.py was bumped to 0.26.0, but the diff sets __version__ = "0.26.2". Please align the version bump statement in the PR description / changelog with the actual version, or adjust the version here if 0.26.0 is the intended target.

__version__ = "0.26.2"

api_app/services/airlock.py:281

• These 503 responses embed the raw exception message from Graph / Event Grid into the HTTP response body (...format(e)). This can leak internal details (endpoints, transient network info, library messages) to API clients. Consider returning a generic 503 detail (or a correlation ID) and keep the full exception details only in logs.

This issue also appears on line 298 of the same file.

        access_service = get_aad_service()
        role_assignment_details = access_service.get_workspace_user_emails_by_role_assignment(workspace)
    except Exception as e:
        logger.exception("Failed to retrieve workspace role assignments from Microsoft Graph")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.GRAPH_ROLE_ASSIGNMENT_ERROR.format(e))

api_app/services/airlock.py:341

• Same as above: returning the raw exception message from the Graph role-assignment lookup in the 503 response can leak internal details. Prefer logging the exception and returning a sanitized value.

    except Exception as e:
        logger.exception("Failed to retrieve workspace role assignments from Microsoft Graph")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.GRAPH_ROLE_ASSIGNMENT_ERROR.format(e))

api_app/services/airlock.py:350

• Same as above: avoid including the raw Event Grid exception message in the HTTP 503 detail. Keep it in logs and return a sanitized value to the caller.

    except Exception as e:
        logger.exception("Failed sending status_changed message")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.EVENT_GRID_PUBLISH_FAILED.format(e))

.flake8:4

• This change globally ignores E231 (whitespace after ',', ':', ';'), which reduces lint signal across the whole repo. Given prior discussion that global config shouldn’t be changed for this workaround, please scope any E231 suppression to the specific offending files/lines (e.g., # noqa: E231 or per-file-ignores) and keep the global ignore list unchanged.

ignore = E501, W503, E231

.github/linters/.flake8:4

• This change globally ignores E231 for the CI linter configuration. As with the root .flake8, this broad suppression will hide real formatting issues repo-wide; please scope any workaround to specific files/lines instead of adding E231 to the global ignore list.

ignore = E501,W503,E231

api_app/services/airlock.py:280

• The HTTP 503 for Graph role-assignment lookup currently includes the raw exception message (via .format(e)). Exception strings from Graph/requests can include internal details (URLs, OData queries, etc.) and are best kept in logs rather than returned to clients.

This issue also appears on line 339 of the same file.

    except Exception as e:
        logger.exception("Failed to retrieve workspace role assignments from Microsoft Graph")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.GRAPH_ROLE_ASSIGNMENT_ERROR.format(e))

api_app/services/airlock.py:301

• The HTTP 503 returned on Event Grid publish failure currently includes the raw exception message (via .format(e)). This can leak internal endpoint/network details to callers; keep the full exception in logs and return a sanitized value instead.

This issue also appears on line 348 of the same file.

    except Exception as e:
        await airlock_request_repo.delete_item(airlock_request.id)
        logger.exception("Failed sending status_changed message")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.EVENT_GRID_PUBLISH_FAILED.format(e))

api_app/_version.py:1

• PR description states api_app/_version.py was bumped to 0.26.0, but this change sets it to 0.26.2. Please align the PR description with the actual version bump (or adjust the version if 0.26.0 was the intended target).

__version__ = "0.26.2"

.flake8:4

• Global flake8 config now ignores E231. This suppresses whitespace-after-',' checks across the whole repo and was previously called out as something we should avoid changing globally. Prefer reverting this change and addressing any false-positives with targeted # noqa: E231 or per-file-ignores (or upgrading pycodestyle) so the rest of the codebase keeps the signal from E231.

[flake8]
# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501, W503, E231

.github/linters/.flake8:4

• The linter config under .github/linters also now ignores E231 globally, which reduces lint coverage for whitespace issues. Consider reverting this and using a more targeted suppression approach (per-file-ignores / # noqa: E231) limited to the specific false-positive cases.

[flake8]
# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501,W503,E231

api_app/services/airlock.py:301

• The 503 response on Event Grid publish failure returns the raw exception message (EVENT_GRID_PUBLISH_FAILED.format(e)). This can leak internal network/service details to clients and can also make the API surface unstable (detail changes by exception type/library version). Prefer a stable client-facing message and log the underlying exception instead (or include a correlation ID).

    except Exception as e:
        await airlock_request_repo.delete_item(airlock_request.id)
        logger.exception("Failed sending status_changed message")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.EVENT_GRID_PUBLISH_FAILED.format(e))

api_app/_version.py:1

• PR description says api_app/_version.py was bumped to 0.26.0, but the code sets 0.26.2. Please align the PR description and version bump (either adjust the description, or update the version to the intended target) to avoid confusion during release notes / traceability.

__version__ = "0.26.2"

api_app/event_grid/helpers.py:46

• raise last_exc # type: ignore can raise None at runtime if the retry loop ever completes without capturing an exception (e.g., future refactors change what gets caught). It also forces a type-ignore. Prefer an explicit guard so the failure mode is deterministic and type-checker friendly.

    raise last_exc  # type: ignore[misc]

api_app/services/airlock.py:281

• The 503 response for Graph role-assignment lookup includes the raw exception string (...format(e)). Exception messages from Graph/requests can include internal URLs, request IDs, or other details that are better kept server-side. Consider returning a generic client message (and keeping full detail in logs), or gating the detailed message behind a debug setting.

This issue also appears on line 298 of the same file.

    try:
        access_service = get_aad_service()
        role_assignment_details = access_service.get_workspace_user_emails_by_role_assignment(workspace)
    except Exception as e:
        logger.exception("Failed to retrieve workspace role assignments from Microsoft Graph")
        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=strings.GRAPH_ROLE_ASSIGNMENT_ERROR.format(e))

api_app/tests_ma/test_api/conftest.py:28

• no_auth_token builds an AuthenticatedUser without an email, so email becomes None. This is inconsistent with the TokenValidator behavior (which normalizes missing email/preferred_username to an empty string) and can reintroduce the same class of failures in tests that exercise payloads expecting a string email.
  api_app/auth/models.py:33
• AuthenticatedUser.email is typed as Optional[str] and defaults to None, but other parts of this PR (and the new TokenValidator tests) rely on email always being a string (empty string allowed) to avoid downstream validation errors. Keeping it optional makes it easy for callers/tests to accidentally reintroduce None emails.

    id: str
    name: str
    email: Optional[str] = None
    roles: Tuple[str, ...] = Field(default_factory=tuple)
    audience: str = ""
    is_workspace_token: bool = False

.flake8:4

• This change globally disables E231 (whitespace after ','), which can hide real formatting issues across the repo. Since this was introduced as a workaround for a pycodestyle edge case, it should be scoped (per-file-ignores / targeted noqa) rather than disabling the rule repo-wide.

# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501, W503, E231

.github/linters/.flake8:4

• Adding E231 to the global ignore list weakens formatting checks for all Python code. If specific files are triggering false positives, prefer scoping the ignore to those files rather than disabling E231 for the entire repo.

# E231 is ignored because pycodestyle on Python 3.12+ mis-tokenises f-string
# contents (commas/colons inside URL, OData and JSON string literals) as code.
ignore = E501,W503,E231

api_app/tests_ma/test_api/conftest.py:28

• The global no_auth_token fixture constructs an AuthenticatedUser without an email, so email defaults to None. Some code paths (e.g. Event Grid airlock notification payload validation) require email to be a string, and this fixture can accidentally reintroduce the previous None-email regression in tests that reuse it.